Skip to content

Conversation

shubham-j-sde
Copy link
Contributor

addresses #285

Feature: Now we can convert our Markdown files to JSX components. that's not all, this also allows to import other JSX components within markdown along with tailwind css and typography support!

@shubham-j-sde
Copy link
Contributor Author

the current docs are just copy of ReadMe file with command line instructions. Docs to be updated after adding UI controller support

@git-hulk git-hulk requested a review from Copilot March 25, 2025 05:57
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds MDX support to the project by introducing new components and configuration changes, as well as updating the docs navigation.

  • Introduces an MDX layout component and a hook for MDX components.
  • Updates configuration to enable MDX support and adjusts the docs link in the banner.
  • Expands license configurations to include markdown files.

Reviewed Changes

Copilot reviewed 7 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
webui/src/pages/mdx-layout.tsx Adds a simple MDX layout component.
webui/mdx-components.tsx Introduces a hook for MDX components with a pass-through return.
webui/src/app/ui/banner.tsx Updates the banner link to point to the new docs route.
.github/licenserc.yaml Adds patterns for markdown (md, mdx) files.
webui/next.config.mjs Configures Next.js to support MDX using an experimental flag.
Files not reviewed (3)
  • webui/package.json: Language not supported
  • webui/src/pages/docs/getting-started.mdx: Language not supported
  • webui/src/pages/docs/supported-commands.mdx: Language not supported
Comments suppressed due to low confidence (3)

webui/mdx-components.tsx:22

  • [nitpick] The function 'useMDXComponents' currently only returns the provided components without modifications. Consider either extending it to merge in custom MDX components or renaming it to better reflect its pass-through behavior.
export function useMDXComponents(components: MDXComponents): MDXComponents {

webui/next.config.mjs:43

  • As 'mdxRs' is an experimental flag, adding a brief comment or reference on its purpose could improve code clarity for future maintainers.
            mdxRs: true,

.github/licenserc.yaml:31

  • The glob pattern '//.md' may be unnecessarily complex; verify if a simpler pattern like '**/.md' would achieve the same matching criteria.
    - '**/**/*.md'

@shubham-j-sde
Copy link
Contributor Author

sorry, i thought **/**/*.mdx should take care of all nested paths. have already tried **/*.mdx
I need to revise regex I suppose, please comment if you have a workaround.

@shubham-j-sde
Copy link
Contributor Author

same license checker gives no errors on my system though

Comment on lines +31 to +32
- '**/**/*.md'
- '**/**/*.mdx'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use standard HTML comment format <!-- ... --> inside markdown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the license checker was using the wrong config #293.

@PragmaTwice
Copy link
Member

PragmaTwice commented Mar 25, 2025

The changes are fine. However, the most important is that, I think we need to put controller docs into our website (kvrocks.apache.org) instead of here. If we also want to put the docs into the web UI, we need a sync mechanism between these two repo. And this will be a bit complicated.

@shubham-j-sde
Copy link
Contributor Author

Actually, I added current docs content as experiment, Once Controller UI is up, I thought it would be feasible for users to have some guide then and there, right?
synchronizing two files will ofcourse be issue, let me check if importing files is an option here, if that sounds right?

@PragmaTwice
Copy link
Member

Maybe we can focus on the controller docs on Kvrocks website first. 🤔

@shubham-j-sde
Copy link
Contributor Author

Sure thing sir! 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants